Skip to content

Add the Touch ID review lessons to AGENTS.md and CONTRIBUTING.md - #14

Merged
maralcbr merged 6 commits into
mainfrom
docs/agents-test-rules
Oct 12, 2026
Merged

maralcbr merged 6 commits into
mainfrom
docs/agents-test-rules

Conversation

@scottjones

@scottjones scottjones commented Oct 5, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Adds two rules and a Tests section to AGENTS.md, from what the Touch ID reviews (#12, omacom/omarchy-mac#694, omacom/omarchy-pkgs#806) kept catching, and states the invitation rule in CONTRIBUTING.md.

Rules:

  • A udev add that must reach a device already present also gets a pacman hook, which exits 0 in a chroot or an unbooted root and never fails the transaction. An add that should only run when the device appears doesn't.
  • An invitation, a prompt the owner sees once such as the post-update fingerprint offer, ships only after everything it leads to is published to the repositories Macs update from. The rule names where the fingerprint pieces come from: Name Touch ID as a fingerprint reader and wake it at boot #12 adds the fingerprint-readers platform file, and Find fingerprint readers a platform names omarchy#14675 makes the runtime's detector read it.

Tests:

  • A test gives the same result on every machine. It never reads the host's platform root, installed omarchy-* commands, /sys or running services, and it stubs external dependencies while keeping the real commands under test. Where the container can verify units, it runs systemd-analyze verify with --root at the staged tree.
  • A new detection path or fallback gets the inverse test.
  • A test that needs a newer runtime moves test/integration/runtime to a commit that has it instead of skipping.
  • A change isn't tested until CI is seen to run it.

Evidence

Documentation only. check-scope, check-architecture-gates and check-fixture-drift pass.

Merge Danger

Door: two-way

Merging makes the invitation rule release policy. CONTRIBUTING.md now states it under Pull requests, next to the cold-boot evidence rule.

Blast Radius: docs

🤖 Generated with Claude Code

Reviews of the Touch ID branches kept finding the same kinds of slip: a
test that passed only because the Mac running it had Omarchy installed, a
detection path that could hide the one before it, an integration test that
skipped silently in CI, a green run that never reached the code, a udev
add rule that missed the upgrade installing it, and a once-only invitation
that could fire before its dependencies shipped. AGENTS.md now says so.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@scottjones
scottjones requested a review from maralcbr as a code owner October 5, 2026 14:22
@maralcbr

maralcbr commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator

@scottjones good idea to write these down; lines 26 and 28 are precise and fit the file. A few wording fixes so agents don't over-apply them:

  • Line 20 (udev add → pacman hook) is too broad. It would also cover 70-omarchy-mac-external-displays.rules, whose add must only run while the boot splash is up, and change actions that must not be replayed. Suggest: "A udev add action that must reach a device already present also gets a pacman hook that does that work on install and upgrade; the hook exits 0 in a chroot or an unbooted root, and never fails the transaction. An add that should only run when the device appears does not."
  • Line 25 ("never read … /sys") reads as a ban on all /sys reads, while existing tests already point copies at /sys fixtures. Suggest "never the host's /usr/share/omarchy-platform, installed omarchy-* commands, /sys or running services: stage into a temporary root and point copies at fixtures."
  • Line 21 ("a step the owner sees only once") also covers first boot, which already belongs to the installer. "An invitation the owner sees only once ships only after everything it leads to is published" says what was meant.
  • Line 27 ("don't skip") sits next to the existing skip when OMARCHY_TEST_RUNTIME is unset. Say it's about the pin: "moves test/integration/runtime to that commit; it does not skip because that pin lacks it."

Wording from Marcelo's review, so agents don't over-apply them: the hook
rule covers only an add that must reach a device already present, and the
hook never fails the transaction; the once-only rule is about invitations,
not first boot; tests may read /sys fixtures, never the host's; and the
pin rule doesn't touch the skip when OMARCHY_TEST_RUNTIME is unset.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@scottjones

Copy link
Copy Markdown
Collaborator Author

@maralcbr thanks, applied all four in 0fc00085.

@maralcbr

maralcbr commented Oct 9, 2026

Copy link
Copy Markdown
Collaborator

Thanks @scottjones, the rules capture real lessons from the Touch ID work. One change before merge:

  • Line 25: please reword to "stub external dependencies, keeping the real commands under test". As written, "stub the runtime's commands" contradicts how the integration tests work.

Smaller things:

  • "Invitation" isn't defined anywhere else in the repo, so an agent can't act on that rule. A short definition (or a pointer to the fingerprint-setup offer it refers to) would fix it.
  • The PR description no longer matches the diff; worth a refresh.

With the line 25 fix in, this is good to merge from my side.

maralcbr and others added 2 commits October 9, 2026 11:32
Marcelo noted "invitation" is defined nowhere else in the repo, so an
agent could not act on the rule. It now names the one-time post-update
fingerprint setup offer and the change that triggers it, a new entry in
fingerprint-readers.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@maralcbr

Copy link
Copy Markdown
Collaborator

Thanks @scottjones, the line 25 fix and the invitation definition are in. A few things left, all wording or timing:

  • Line 21 names omarchy-hw-fingerprint and fingerprint-readers, which only arrive with Name Touch ID as a fingerprint reader and wake it at boot #12. Either merge this after Name Touch ID as a fingerprint reader and wake it at boot #12, or say where they come from. "Published" is also open to reading as merged; suggest "published to the repositories Macs update from".
  • Line 25: the systemd-analyze verify --root sentence is unconditional, but existing tests differ: omarchy-mac-boot/test/mac-external-displays-test.sh:170 verifies without --root and skips when the container cannot verify units, and mac-hid-initramfs-test.sh:353 uses --root on the extracted initramfs. Suggest "where the container can verify units, run it with --root at the staged tree".
  • Line 27 (nit): "moves test/integration/runtime to that commit" can read as the exact commit that gained the feature, while the pin is a reviewed commit (today #13362's head). "to a commit that has it" is clearer.
  • The PR description still shows the old diff; worth a refresh.

Line 21 is also a release gate not written down in CONTRIBUTING.md; I'm happy to adopt it, just flagging that merging this makes it policy.

scottjones and others added 2 commits October 9, 2026 21:18
From Marcelo's review: the invitation rule now says where its names come
from (omarchy-hw-fingerprint, the runtime's detector, reads
fingerprint-readers, the platform file omarchy-mac ships in #12), and
"published" means published to the repositories Macs update from.
systemd-analyze verify --root applies where the container can verify
units, at whatever tree is staged. The runtime pin moves to a commit
that has what a test needs, not the exact commit that added it.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ONTRIBUTING.md

AGENTS.md said #12 brought both the platform file and the detector that
reads it. #12 adds fingerprint-readers; omacom/omarchy#14675 teaches the
runtime's omarchy-hw-fingerprint to read it. CONTRIBUTING.md now states
the release rule for one-time prompts beside the cold-boot evidence rule,
so merging the AGENTS.md rule doesn't make policy the contributor guide
leaves out.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@scottjones scottjones changed the title Add test rules learned from the Touch ID work to AGENTS.md Add the Touch ID review lessons to AGENTS.md and CONTRIBUTING.md Oct 12, 2026
@scottjones

Copy link
Copy Markdown
Collaborator Author

@maralcbr thanks for this, and sorry for the slow reply. All five are in:

Could you take another look when you get a minute? It still needs an approval before it can merge.

@maralcbr
maralcbr merged commit 654d21b into main Oct 12, 2026
6 checks passed

@maralcbr maralcbr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @scottjones, all of it is in: the source of the fingerprint names, "published to the repositories Macs update from", the conditional --root, "a commit that has it", and the refreshed description. Good to merge from my side.

  • Nit, line 21: the last sentence reads as true today, but fingerprint-readers only arrives with #12, and the pinned omarchy-hw-fingerprint doesn't read it until omacom/omarchy#14675 lands. Either merge this after #12, or something like "#12 adds fingerprint-readers to omarchy-mac, and omacom/omarchy#14675 teaches the runtime's detector, omarchy-hw-fingerprint, to read it."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants